Security: use x-pack config files when present#33688
Conversation
In versions prior to 6.3, x-pack configuration files were kept within a x-pack subdirectory. In 6.3 these files were moved to the config directory with a backwards compatibilty layer to look for the files in the old location if file does not exist in the new location. This works for files that are not pre-created for the user, but in the case of security there are several files created by default. This change updates the security code to check if the file in the new location is a default file that has not been modified. If it is a default file, the code will also see if a file exists in the pre 6.3 location and use it instead. Closes elastic#33464
jaymode
added
>bug
v7.0.0
:Security/Security
|
Pinging @elastic/es-security |
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
LGTM, a couple minor suggestions
x-pack/plugin/security/build.gradle
Outdated
| } | ||
|
|
||
| sourceSets.main.resources { | ||
| srcDir 'src/main/config' |
There was a problem hiding this comment.
Maybe place this in a subdir under resources, so we don't pollute the top level?
| try { | ||
| in = XPackPlugin.class.getResourceAsStream("/" + name); | ||
| if (in != null) { | ||
| List<String> defaultLines = Streams.readAllLines(in); |
There was a problem hiding this comment.
Why not just compare bytes, instead of interpreting into character strings?
|
My concern (but I don't have a fantastic suggestion instead) is that this puts users at risk of accidentally modifying the wrong file and having behaviours change on them. Specifically:
I don't particularly like that behaviour, but as I said I don't have much else to suggest except maybe we can fix the "silently" bit. |
|
I'd be happy with adding that logging in a separate PR if we agree it's the best mitigation for that sort of scenario. |
|
@tvernum You have some good points.
This sounds like a good solution. |
|
@rjernst do you mind taking another look? I've updated to address the new log message and your feedback. |
|
|
||
| private static final Logger logger = Loggers.getLogger(Security.class); | ||
| private static final Logger logger = LogManager.getLogger(Security.class); | ||
| private static DeprecationLogger deprecationLogger = new DeprecationLogger(logger); |
There was a problem hiding this comment.
nit: these are statics, so they should be named with all caps?
| throw new UncheckedIOException(e); | ||
| } finally { | ||
| if (in != null) { | ||
| IOUtils.closeWhileHandlingException(in); |
There was a problem hiding this comment.
What's the reason try-with-resources can't be used?
There was a problem hiding this comment.
I forgot that try-with-resources can handle nulls. Switched.
In versions prior to 6.3, x-pack configuration files were kept within a x-pack subdirectory. In 6.3 these files were moved to the config directory with a backwards compatibilty layer to look for the files in the old location if file does not exist in the new location. This works for files that are not pre-created for the user, but in the case of security there are several files created by default. This change updates the security code to check if the file in the new location is a default file that has not been modified. If it is a default file, the code will also see if a file exists in the pre 6.3 location and use it instead. Closes #33464
5 participants
In versions prior to 6.3, x-pack configuration files were kept within a
x-pack subdirectory. In 6.3 these files were moved to the config
directory with a backwards compatibilty layer to look for the files in
the old location if file does not exist in the new location. This works
for files that are not pre-created for the user, but in the case of
security there are several files created by default. This change
updates the security code to check if the file in the new location is
a default file that has not been modified. If it is a default file, the
code will also see if a file exists in the pre 6.3 location and use it
instead.
Closes #33464